Add block/boolean NSPredicates, NSCompoundPredicate, and NSArray predicate method #127
Conversation
| // | ||
| // Created by Kevin Lundberg on 12/11/15. | ||
| // Copyright © 2015 Apple. All rights reserved. | ||
| // |
There was a problem hiding this comment.
You should look at another Swift file in this project and add a correct legal notice.
There was a problem hiding this comment.
Ah yeah, missed that, good call. I'll handle that in the morning.
Kevin Lundberg
On Dec 11, 2015, at 1:44 AM, Thi notifications@github.com wrote:
In TestFoundation/TestNSCompoundPredicate.swift:
@@ -0,0 +1,87 @@
+//
+// TestNSCompoundPredicate.swift
+// Foundation
+//
+// Created by Kevin Lundberg on 12/11/15.
+// Copyright © 2015 Apple. All rights reserved.
+//
You should look at another Swift file in this project and add the same legal notice.—
Reply to this email directly or view it on GitHub.
|
Patch looks good, but the compiler crashes on linux when compiling NSCompoundPredicate. =( I'm investigating, but in the meantime can you rebase so that we don't have commits with the wrong license in them in the project history? |
|
Actually there is some confusion here about required/convenience initializers; Darwin Foundation doesn't correctly mark them so there is nothing to go on except to look at what happens there. I think the block initializer may also be a problem, since technically it's a factory method and returns an instance of NSBlockPredicate instead of storing the block in the NSPredicate superclass. |
|
To work around the factory method problem, I guess NSPredicate just has to gain knowledge of these modes (block, permanent true/false, etc) instead of using a subclass. In any case I can't seem to figure out how to work around the compiler crash at the moment. |
|
I'm setting up a linux env to test on so I can try to figure out what's going on too. I'll rebase later tonight as well |
klundberg
force-pushed
the
master
branch
2 times, most recently
from
1d2c6bc
to
31d8641
Compare
|
Just rebased against latest master. I also rewrote NSPredicate a little bit to use a private enum to represent the different predicate types in a more idiomatic swift way. This had led to a small change on NSCompoundPredicate, which may compile on linux now (I can't seem to build swift on ubuntu 15.10 due to some odd linker issues I have not yet resolved so I can't verify this). On a slightly unrelated note (though present in the PR) I encountered an API issue here (also with Darwin foundation) which I notified the swift-corelibs-dev list about, and I'm prepared to make a proposal to swift-evolution if necessary to fix it, unless it's just a nullability bug on apple's end: https://lists.swift.org/pipermail/swift-corelibs-dev/Week-of-Mon-20151214/000170.html |
|
I've finally gotten a working linux environment I can test in. I figured out the compiler bug; in NSCompoundPredicate if the subpredicates array is of type [NSPredicate] the compiler does not crash; there must be some issue compiling code that does implicit upcasts to [AnyObject]. I will update SR-204 with this info as well, and I've logged a radar (23894694) to have this fixed in Darwin Foundation. I''ll make this change in another commit shortly so that it will be merge-able. |
|
changing the test to func test_filterNSArray() {
let predicate = NSPredicate { (obj, bindings) -> Bool in
return (obj as? NSString).map({ $0.length <= 2 }) == true
}
let array = NSArray(array: ["1".bridge(), "12".bridge(), "123".bridge(), "1234".bridge()])
let filteredArray = array.filteredArrayUsingPredicate(predicate).map { $0 as! NSObject }
XCTAssertEqual(["1".bridge(), "12".bridge()] as [NSObject],
filteredArray)
}works around the compiler crash |
|
I didn't encounter a crash in the tests; by changing the type of the array I was also able to work around the crash. I havent used more recent builds of swift though so maybe something else was introduced that broke it there. If I have time this weekend I'll look at it. |
|
Thanks for this pull request!! swift-corelibs-xctest requires a usable Is there any way we can help get this pull request merged? Is it possible that rebasing and testing with the latest Swift compiler may resolve the crash? |
|
I can certainly rebase with master and test again in the next couple of days. If the API changes to this are still necessary to work around the crash (changes i would argue are warranted even if the crash is fixed), is it OK to leave them in? |
|
Let's get it rebased first, then we can run it through CI and see if the crash is fixed at least. |
|
Just rebased, and undid the change that caused the crash on linux. I don't have my linux environment handy, but it compiles on the latest development snapshot toolchain on os x. |
|
@swift-ci Please test |
|
Yup, looks like a segfault in the compiler. Maybe put the workaround back in? |
|
I made a change that might work, but I can't test in linux right now. This change keeps the external API in sync with Darwin Foundation but tries to explicitly cast to [AnyObject] from [NSPredicate]. Could someone kick off a ci test? If this fails, then I can try changing the subpredicates property back to [NSPredicate] but that will bring the APIs out of sync again, unless the radar I filed awhile back (rdar://23894694) will be addressed this summer. |
|
@swift-ci please test |
|
Looks like it's no good, there's still a segfault. I have access to a Linux machine, I'll try to find a workaround on my end as well. |
|
Thanks! If nothing else works I will make the change that alters the API.
|
|
@klundberg What change would that be? Sorry if it's already somewhere above, I'm having trouble finding it. |
|
I had changed the type of the
|
|
There is a current open bug to rename that to be properly generically qualified so; lets err on the side of ambition and claim that will be resolved in favor of changing it. The differential shouldn't hold this up (just is worth noting) |
| return subpredicates.reduce(false, combine: { $0 || $1.evaluateWithObject(object, substitutionVariables: bindings) }) | ||
| return subpredicates.reduce(false, combine: { | ||
| $0 || $1.evaluateWithObject(object, substitutionVariables: bindings) | ||
| }) |
There was a problem hiding this comment.
Using reduce doesn't maintain the short-circuiting behavior that the ObjC Foundation version exhibits. As a test, the following will not crash, indicating that NSCompoundPredicate is short-circuiting predicate evaluation once it knows that it will return false:
let bOK = NSPredicate(value: false)
let bCrash = NSPredicate(block: { _ in
fatalError("whoops")
return false
})
let both = NSCompoundPredicate(andPredicateWithSubpredicates: [bOK, bCrash])
let eval = both.evaluateWithObject("hi") // evaluates to false
There was a problem hiding this comment.
I don't see how that's true. I added test cases for both "and" and "or predicates based on your example code here, and I did not get any test failures or crashes. The && and || operators short circuit properly on their own, and I don't see how reduce() could alter that behavior.
|
@phausler thanks for the go-ahead. I was hoping this would be the case since it's clearly the better API :) |
|
Cleaned up my commits, undid some testing changes I forgot to undo before committing last time, and rebased! |
|
Cool, let's run this through the tests one more time. |
|
@swift-ci please test |
|
Looks like a different crash than before. I'll fix it up tonight or tomorrow and try to get my linux VM in a state that I can test with myself. |
|
|
|
That might work; I came up with what I think is a simpler approach though: test NSArray objects for equality, not swift arrays. No need for fancy casting workarounds in that case.. |
|
Simpler in this case is better. Especially when we have a persistent compiler crash lurking in the shadows. |
|
@swift-ci Please test |
FoundationXML: Adoption
I added some of the NSPredicate implementation, including block predicates and true/false predicates, NSCompoundPredicate (which should be complete I believe), and an implementation to NSArray.filteredArrayUsingPredicate.
My current simple NSPredicate work only works with nil for substitution bindings since I don't fully understand how those work yet, or if they're even applicable to block based predicates.
"Not" compound predicates trigger a precondition if the array of subpredicates is empty. This is similar to what happens in Foundation on Apple platforms.